-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550
Conversation
The original PR is labelled The commit that landed on master 756a34e lands cleanly on 9.x, is there a reason for the explicit backport? Is there a difference between the changes? |
0e105ae
to
bc156da
Compare
AFAIK it's because there was one reverted commit here, see #14901 (comment) (this comment also explains why I opened this backport PR). |
b9b5810
to
6edf952
Compare
610fdb8
to
2c9a85d
Compare
@MylesBorins I added two related PRs to this backport, since they improve these changes. |
Shutting down the connection is what `_final` is there for. PR-URL: nodejs#18608 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Currently the following deprecation warning is produced when compiling node_perf.cc: ./src/node_perf.cc:91:11: warning: 'MakeCallback' is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations] node::MakeCallback(env->isolate(), ^ ../src/node.h:172:50: note: 'MakeCallback' has been explicitly marked deprecated here NODE_EXTERN v8::Local<v8::Value> MakeCallback( ^ 1 warning generated. This commit adds an async_context to the call and checks the maybe result. PR-URL: nodejs#18877 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
PR-URL: nodejs#18908 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Make sure that inspector ports in cluster are inside the valid range: `[1024, 65535]`. Fixes flaky `test-inspector-port-zero-cluster`. PR-URL: nodejs#18696 Fixes: nodejs#18303 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#18607 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18816 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This fixes a few rules by making sure the input is actually ready to be checked. Otherwise those can throw TypeErrors or result in faulty error messages. PR-URL: nodejs#18853 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: nodejs#16864 PR-URL: nodejs#17015 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Set `socket._httpMessage` to `null` before emitting the `'connect'` or `'upgrade'` event. PR-URL: nodejs#18865 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#18859 Reviewed-By: James M Snell <jasnell@gmail.com>
When create a nest repl, will register `Runtime.executionContextCreated` listener to the inspector session.This patch will fix listener repeatedly register. PR-URL: nodejs#18881 Fixes: nodejs#18284 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Remove an erroneous CHECK that asserted the persistent object's internal field pointer still pointed to a valid object. If ClearWrap() has been called, the field pointer equals nullptr and that is expected behavior. PR-URL: nodejs#18898 Fixes: nodejs#18256 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matheus Marchini <matheus@sthima.com>
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: nodejs#18334 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: nodejs#18334 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
fafff2f
to
5329a07
Compare
Commit added to avoid conflict with #18954: 5329a07cbc73d702ebf71400747530bf8ed9d6b2 |
This commit add SetUpTestCase and TearDownTestCase functions that will be called once per test case. Currently we only have SetUp/TearDown which are called for each test. This commit moves the initialization and configuration of Node and V8 to be done on a per test case basis, but gives each test a new Isolate. Backport-PR-URL: nodejs#18957 PR-URL: nodejs#18558 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Share `process` through the module wrapper rather than relying on nobody messing with `global.process`. Backport-PR-URL: nodejs#19006 PR-URL: nodejs#17198 Fixes: nodejs#6802 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Do not share unnecessary information about nextTick state between JS & C++, instead only track whether a nextTick is scheduled or not. Turn nextTickQueue into an Object instead of a class since multiple instances are never created. Other assorted refinements and refactoring. Backport-PR-URL: nodejs#19006 PR-URL: nodejs#17738 Reviewed-By: Anna Henningsen <anna@addaleax.net>
When a process encounters a _fatalException that is caught, it should schedule execution of nextTicks but not in an arbitrary place of the next Immediates queue. Instead, add a no-op function to the queue that will ensure processImmediate runs, which will then ensure that nextTicks are processed at the end. Backport-PR-URL: nodejs#19006 PR-URL: nodejs#17841 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Make setImmediate() immune to `process` global tampering by removing the dependency on the `process._immediateCallback` property. Backport-PR-URL: nodejs#19006 PR-URL: nodejs#17736 Fixes: nodejs#17681 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#19006 PR-URL: nodejs#17881 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
If an error is encountered during the processing of Immediates, schedule the remaining queue to finish after all error handling code runs (if the process is still alive to do so). The new changes make the Immediates error handling behaviour entirely deterministic and predictable, as the full queue will be flushed on each Immediates cycle, regardless of whether an error is encountered or not. Currently this processing is scheduled for nextTick which can yield unpredictable results as the nextTick might happen as early as close callbacks phase or as late as after the next event loop turns Immediates all fully processed. The latter can result in two full cycles of Immediates processing during one even loop turn. The current implementation also doesn't differentiate between Immediates scheduled for the current queue run or the next one, so Immediates that were scheduled for the next turn of the event loop, will process alongside the ones that were scheduled for the current turn. Backport-PR-URL: nodejs#19006 PR-URL: nodejs#17879 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Refactor Immediates handling to allow for them to be unrefed, similar to setTimeout, but without extra handles. Document the new `immediate.ref()` and `immediate.unref()` methods. Add SetImmediateUnref on the C++ side. Backport-PR-URL: nodejs#19006 PR-URL: nodejs#18139 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Before these changes, only V8 added postmortem metadata to Node's binary, limiting the possibilities for debugger's developers to add some features that rely on investigating Node's internal structures. These changes are first steps towards empowering debug tools to navigate Node's internal structures. One example of what can be achieved with this is shown at nodejs/llnode#122 (a command which prints information about handles and requests on the queue for a core dump file). Node postmortem metadata are prefixed with nodedbg_. This also adds tests to validate if all postmortem metadata are calculated correctly, plus some documentation on what is postmortem metadata and a few care to be taken to avoid breaking it. Ref: nodejs/llnode#122 Ref: nodejs/post-mortem#46 PR-URL: nodejs#14901 Refs: nodejs/post-mortem#46 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#18530 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. PR-URL: nodejs#18653 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Currently the cctest target depend on the node_core_target_name target. But it is the node_lib_target_name target that compiles the sources now which means that if a source file in src is updated the cctest executable will not be re-linked against it, but will remain unchanged. The code will still be compiled, just not linked which means that if you are debugging you'll not see the changes and also a warning will be displayed about this issue. This commit changes the cctest target to depend on node_lib_target_name. PR-URL: nodejs#18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
5329a07
to
686a66d
Compare
Resolved conflict. New CI: https://ci.nodejs.org/job/node-test-pull-request/13379/ |
Another CI (just to be sure): https://ci.nodejs.org/job/node-test-pull-request/13380/ |
2177138
to
319cd44
Compare
Before these changes, only V8 added postmortem metadata to Node's binary, limiting the possibilities for debugger's developers to add some features that rely on investigating Node's internal structures. These changes are first steps towards empowering debug tools to navigate Node's internal structures. One example of what can be achieved with this is shown at nodejs/llnode#122 (a command which prints information about handles and requests on the queue for a core dump file). Node postmortem metadata are prefixed with nodedbg_. This also adds tests to validate if all postmortem metadata are calculated correctly, plus some documentation on what is postmortem metadata and a few care to be taken to avoid breaking it. Ref: nodejs/llnode#122 Ref: nodejs/post-mortem#46 Backport-PR-URL: #18550 PR-URL: #14901 Refs: nodejs/post-mortem#46 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. Backport-PR-URL: #18550 PR-URL: #18653 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Currently the cctest target depend on the node_core_target_name target. But it is the node_lib_target_name target that compiles the sources now which means that if a source file in src is updated the cctest executable will not be re-linked against it, but will remain unchanged. The code will still be compiled, just not linked which means that if you are debugging you'll not see the changes and also a warning will be displayed about this issue. This commit changes the cctest target to depend on node_lib_target_name. Backport-PR-URL: #18550 PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
I rebased a commit out of v9.x-staging that was causing failures on Windows in CI. Landed on v9.x-staging, post-land-CI (because I’m done for tonight): https://ci.nodejs.org/job/node-test-commit/16514/ |
Before these changes, only V8 added postmortem metadata to Node's binary, limiting the possibilities for debugger's developers to add some features that rely on investigating Node's internal structures. These changes are first steps towards empowering debug tools to navigate Node's internal structures. One example of what can be achieved with this is shown at nodejs/llnode#122 (a command which prints information about handles and requests on the queue for a core dump file). Node postmortem metadata are prefixed with nodedbg_. This also adds tests to validate if all postmortem metadata are calculated correctly, plus some documentation on what is postmortem metadata and a few care to be taken to avoid breaking it. Ref: nodejs/llnode#122 Ref: nodejs/post-mortem#46 Backport-PR-URL: #18550 PR-URL: #14901 Refs: nodejs/post-mortem#46 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. Backport-PR-URL: #18550 PR-URL: #18653 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Currently the cctest target depend on the node_core_target_name target. But it is the node_lib_target_name target that compiles the sources now which means that if a source file in src is updated the cctest executable will not be re-linked against it, but will remain unchanged. The code will still be compiled, just not linked which means that if you are debugging you'll not see the changes and also a warning will be displayed about this issue. This commit changes the cctest target to depend on node_lib_target_name. Backport-PR-URL: #18550 PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Backport of #14901, #18530 and #18653 to v9.x.
Even though no manual changes were required while cherry-picking, I'm backporting to avoid problems with a reverted commit present in #14901 (see #14901 (comment)). I'm also backporting #18530 and #18653 since both PRs are improvements of this feature.
make -j4 test
(UNIX)Affected core subsystem(s)
src, test